-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement pending join pool #1587
base: dev
Are you sure you want to change the base?
Conversation
(NodeJoinsTheCluster.getClass, 209, CompatibleSerializer), | ||
(NodeLeavesTheCluster.getClass, 210, CompatibleSerializer), | ||
(NodeKickedOutByHealthcheck.getClass, 211, CompatibleSerializer), | ||
(classOf[NextActiveNodes], 212, CompatibleSerializer), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not classOf[]
as it was used before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, you used Foo.getClass
instead of classOf[Foo]
as it was used almost everywhere else. that's why I'm asking ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok you mean e.g. the line 121. That's how we got the class for the case objects. classOf[] doesn't work for case objects AFAIR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup. Ok, didn't know
lastSnapshot: String, | ||
checkpointBlocks: Seq[String], | ||
publicReputation: SortedMap[Id, Double], | ||
nextActiveNodes: NextActiveNodes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if we will make a rollback and there are no such nodes in the cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rollback case may not be handled yet. Essentially during rollback the NextActiveNodes should be ignored and we should start from the initial nodes - currently based on the whitelisting file. I mentioned it on the daily sometime ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it will be then NextActiveNodes(Set.empty, Set.empty)
but @TheMMaciek please confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During rollback these will be set to some nodes, but we need to ignore them during rollback and use the initialFullNodes -> currently these are first 3 nodes in the whitelisting file. The initial nodes logic can be changed of course, especially if we will remove whitelisting.
@@ -51,7 +51,7 @@ class ConsensusManager[F[_]: Concurrent: ContextShift: Timer]( | |||
nodeId: Id, | |||
keyPair: KeyPair, | |||
checkpointsQueueInstance: DataResolverCheckpointsEnqueue[F] | |||
) { | |||
)(implicit F: Concurrent[F], CS: ContextShift[F], T: Timer[F]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need F only explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we wanted to have it uniform, so I moved all of them to this notation. I can move only what I need.
def getActivePeersIds(withSelfId: Boolean = false): F[Set[Id]] | ||
def getActivePeers: F[Map[Id, PeerData]] | ||
def getActiveLightPeersIds(withSelfId: Boolean = false): F[Set[Id]] | ||
def getActiveLightPeers(): F[Map[Id, PeerData]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getActiveLightPeers
instead of getActiveLightPeers()
to be consistent with the other methods. if it makes an action, then ()
. if not and it is only a get, no ()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly what happens, getActiveLightPeers() performs an action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what? how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can look it up, but essentially what it does is it takes the list of lightPeersIds, and fetches their PeerData from the list of all peers. There is some more logic happening for this case and I'm not sure it should stay their inside a storage, but that's how I rebased it initially to test if the implementation still works. The thing is this function is needed in different places around the code base, it was previously in Cluster class, now it's in storage and it kind of clashes with the idea that storage should only store data so possibly I will need to improve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but all in all, it's get active light peers
. doesn't matter how it works under the hood. if it takes it from the database, API or Mars ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah kind of makes sense ;). I remember we discussed that only for a simple getter it should be parameterless (just getting the value) but here much more happens actually. Though it's true that behind the interface it's not distinguishable. That said I can change it to parameterless and we can assume that every get function with no parameters should be in parameterless notation, or we could drop that convention and use empty parameter list simply everywhere.
def getActiveFullPeersIds(withSelfId: Boolean = false): F[Set[Id]] | ||
def getActiveFullPeers(): F[Map[Id, PeerData]] | ||
def setActiveFullPeers(peers: Set[Id]): F[Unit] | ||
def isAnActivePeer: F[Boolean] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is node
and cluster
. Node storage is about self-knowledge. Cluster storage about the other peers.
Here, isAnActivePeer
takes no parameter so I think it should belong to node storage, not cluster storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it still can be moved to where it fits more.
// TODO: notify new light nodes to join the pool also and update your light nodes list | ||
// TODO: we need a consensus over new light and full nodes between current active full nodes | ||
private def notifyNewActivePeersAndLeaveThePool(): F[Unit] = | ||
for { | ||
_ <- logger.debug(s"Notifying new active peers to join the active pool!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is a part of redownload service
? It should be separated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it can be moved. Redownload flow seemed like the best place to put it initially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo it should be in a separate service. and the same for storage related to joining pool
|
||
private val peers: Ref[F, Map[Id, PeerData]] = Ref.unsafe(Map.empty[Id, PeerData]) | ||
|
||
// TODO: move initialization of the refs below to somewhere outside - Cluster class startup? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in startup flow currently. It was initialized during Cluster class startup till refs where moved to storage classes and because clusterStorage and nodeStorage are only aware of themselves the initial state (like initial full nodes set based on whitelisting file) can't be set during storage initial creation. I did put keyPair or nodeId in some storages but I'm not convinced it should be like that, so I still plan to move these outside of storage and handle logic that needs these parameters differently.
40f825b
to
68b663d
Compare
No description provided.